Add custom withdrawal transaction fee options on Send funds (BTC) #5312

BtcContributor · 2021-03-15T14:09:42Z

Now when you withdraw BTC funds you will see this:

Note that I removed the same option from Settings as it is not useful anymore to me.
I think it would be great to compact even more the General Preferences but I am not sure how to do that.

Issue #5304 will be fixed once this will be approved. (@wallclockbuilder is now taking care of that)

…e it from settings

wallclockbuilder · 2021-03-15T16:14:25Z

@BtcContributor Please ask me before submitting PRs for my issues. We are duplicating work.

BtcContributor · 2021-03-15T16:19:09Z

@wallclockbuilder Sorry but I did not understand that you were working on a fix.
Usually who opens issues does not automatically fix them unless he claims them as you already did on other issues that have been explicitly assigned to you.
Anyway, sorry about that.

Please test it and let me know if you have some feedback.

wallclockbuilder · 2021-03-15T16:23:10Z

Whoever opens the issue is by default responsible for implementation unless it has been assigned to someone else. No one is allowed to presume someone else will do the legwork to make their issue fixed.

This has not been assigned to anyone so by default I, the issue author, is responsible for the implementation.

wallclockbuilder

Add tests so we know its working and also get notified if something breaks this in the future.

wallclockbuilder · 2021-03-15T16:31:01Z

desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java

@@ -221,6 +228,52 @@ public void initialize() {
        withdrawMemoTextField = addTopLabelInputTextField(gridPane, ++rowIndex,
                Res.get("funds.withdrawal.memoLabel", Res.getBaseCurrencyCode())).second;

+        Tuple3<Label, InputTextField, ToggleButton> customFeeTuple = addTopLabelInputTextFieldSlideToggleButton(gridPane, ++rowIndex,


You're reorganising 40 lines of code(more than 7lines i.e. readable). Where are the unit tests and/or end-to-end tests for this?

That way we know it works all the time, and we also know when it gets accidentally broken by some other feature.

I am just reusing most of the code that was already written for Settings.
Unit tests were not available and I would prefer that someone more experienced than me could take that, if needed.

ripcurlx · 2021-03-17T14:30:38Z

Whoever opens the issue is by default responsible for implementation unless it has been assigned to someone else. No one is allowed to presume someone else will do the legwork to make their issue fixed.

This has not been assigned to anyone so by default I, the issue author, is responsible for the implementation.

This isn't the default behavior how this worked so far. If someone is interested in working on an issue within the Bisq repository, she should do so by posting a comment there, even it is their own issue (actually it is the rare case, that the same person who created an issue also fixed it in the past). As soon as she is assigned, everyone knows who is working on this issue. Of course everyone can work as she pleases, but it is advised to get feedback if an issue is within the current priorities and has a high chance to get compensated by the DAO before start working on it.

wallclockbuilder · 2021-03-17T17:27:48Z

@ripcurlx Please assign the issue related to this PR to @BtcContributor. I'll express interest in my issues that I want to submit a PR for.

sqrrm

I like the idea of moving settings closer to where they're used, but I don't think it's good to remove the choice of fee for BSQ transactions.

sqrrm · 2021-03-19T11:39:51Z

desktop/src/main/java/bisq/desktop/main/settings/preferences/PreferencesView.java

-    private InputTextField transactionFeeInputTextField, ignoreTradersListInputTextField, ignoreDustThresholdInputTextField,
+    private InputTextField ignoreTradersListInputTextField, ignoreDustThresholdInputTextField,


Removing this field means you cannot set a manual fee for BSQ transactions.

You are right @sqrrm. I did that as there is this issue #5304 that is similarly taking care of BSQ and that would be next to the merge of this one. However I could restore that line as you said if you think it is better that way.

As it is not sure how quickly #5304 will be implemented I think it would be good to leave this duplication for now and remove it with the refactoring for BSQ. WDYT?

Agree with you. Just pushed this change.

You just restored the reference for the InputTextField, but still removed the creation and functionality of it in Preferences. What I meant was to restore the functionality as it was before, but extract the UI element creation and most of it's functionality to FormBuilder and GUIUtil to prevent unnecessary duplication. Afterwards most of it can be re-used or adapted for the BSQ custom fee setting as well and removed from Preferences completely.

ripcurlx

NACK - Please see my comment: #5312 (comment)

BtcContributor · 2021-04-19T15:00:45Z

I restored the functionality in Settings and just stated more explicitly that now the option in Settings is for BSQ, as for Btc will be used the new one in Funds -> Send funds instead.

As regard the repackage for avoiding duplicates, I talked to @jmacxx and he would do that after this PR is merged as honestly I am not sure to be able to do that properly.

ripcurlx

utACK

mjtic12 · 2021-05-09T22:44:31Z

@BtcContributor @wallclockbuilder
Just had a strange behaviour on using this feature.
I entered 10 sat/vb as custom value for the transaction fee.
But when I checked my transaction in mempool, it displayed 47,1 stat/vb.

If i remember right, it could be that, in the summary the calculated amount for the complete transaction was that one with 47sats v/b. but i did not realised it, that the amount was to high.

BtcContributor · 2021-05-16T12:19:05Z

@BtcContributor @wallclockbuilder
Just had a strange behaviour on using this feature.
I entered 10 sat/vb as custom value for the transaction fee.
But when I checked my transaction in mempool, it displayed 47,1 stat/vb.

If i remember right, it could be that, in the summary the calculated amount for the complete transaction was that one with 47sats v/b. but i did not realised it, that the amount was to high.

Hi, could you please open a separate Github issue detailing each step, while attaching logs and screenshot, please?

I am in particular interested in this: "If i remember right, it could be that, in the summary the calculated amount for the complete transaction was that one with 47sats v/b. but i did not realised it, that the amount was to high."

So a screenshot of this should be useful.

Thanks!

mjtic12 · 2021-05-16T13:19:29Z

Sorry, unfortunately I don't took screenshots during the process. But today there was another user with the same problem (see support-channel) and he did screens. Maybe he can provide it.

BtcContributor · 2021-05-17T13:51:52Z

Sorry, unfortunately I don't took screenshots during the process. But today there was another user with the same problem (see support-channel) and he did screens. Maybe he can provide it.

I just checked that on Mainnet and it seems to work as expected.

I am waiting for some more details by you and keyburner to help me reproduce the bug.

Did you maybe use Use custom inputs when sending funds? See this, maybe it could be related: #5483. This will ship soon on v1.6.5.

Please always mention me when you want my attention, while opening a new Github issue and not commenting on an already merged PR. Thanks.

BtcContributor added 2 commits March 15, 2021 15:03

Add custom withdrawal transaction fee options on Send funds and remov…

ebd98f8

…e it from settings

Remove redundant feePerVbyte variable

5604369

wallclockbuilder suggested changes Mar 15, 2021

View reviewed changes

sqrrm reviewed Mar 19, 2021

View reviewed changes

BtcContributor mentioned this pull request Mar 30, 2021

For Cycle 23 bisq-network/compensation#809

Closed

Restore field used to set a manual fee for BSQ transactions

bf2c356

BtcContributor requested a review from ripcurlx April 13, 2021 10:33

ripcurlx suggested changes Apr 14, 2021

View reviewed changes

BtcContributor force-pushed the sendcustomfee branch from b144c3d to bf2c356 Compare April 19, 2021 14:23

Restore manual fees for BSQ transactions in Settings

d235316

BtcContributor requested a review from ripcurlx April 19, 2021 16:59

ripcurlx approved these changes Apr 20, 2021

View reviewed changes

ripcurlx added this to the v1.6.3 milestone Apr 20, 2021

ripcurlx merged commit fbd7c9f into bisq-network:master Apr 20, 2021

BtcContributor deleted the sendcustomfee branch April 20, 2021 08:36

BtcContributor mentioned this pull request Apr 28, 2021

Unhide custom mining fee option for sending BSQ #5304

Closed

ghost mentioned this pull request Jan 13, 2022

On "Send Funds" screen add ability to change withdrawal trans fee sats rate #5199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom withdrawal transaction fee options on Send funds (BTC) #5312

Add custom withdrawal transaction fee options on Send funds (BTC) #5312

BtcContributor commented Mar 15, 2021 •

edited

Loading

wallclockbuilder commented Mar 15, 2021

BtcContributor commented Mar 15, 2021

wallclockbuilder commented Mar 15, 2021

wallclockbuilder left a comment

wallclockbuilder Mar 15, 2021

wallclockbuilder Mar 15, 2021

BtcContributor Mar 15, 2021 •

edited

Loading

ripcurlx commented Mar 17, 2021

wallclockbuilder commented Mar 17, 2021

sqrrm left a comment

sqrrm Mar 19, 2021

BtcContributor Mar 19, 2021

ripcurlx Apr 13, 2021

BtcContributor Apr 13, 2021

ripcurlx Apr 14, 2021

ripcurlx left a comment

BtcContributor commented Apr 19, 2021 •

edited

Loading

ripcurlx left a comment

mjtic12 commented May 9, 2021

BtcContributor commented May 16, 2021

mjtic12 commented May 16, 2021

BtcContributor commented May 17, 2021 •

edited

Loading

		private InputTextField transactionFeeInputTextField, ignoreTradersListInputTextField, ignoreDustThresholdInputTextField,
		private InputTextField ignoreTradersListInputTextField, ignoreDustThresholdInputTextField,

Add custom withdrawal transaction fee options on Send funds (BTC) #5312

Add custom withdrawal transaction fee options on Send funds (BTC) #5312

Conversation

BtcContributor commented Mar 15, 2021 • edited Loading

wallclockbuilder commented Mar 15, 2021

BtcContributor commented Mar 15, 2021

wallclockbuilder commented Mar 15, 2021

wallclockbuilder left a comment

Choose a reason for hiding this comment

wallclockbuilder Mar 15, 2021

Choose a reason for hiding this comment

wallclockbuilder Mar 15, 2021

Choose a reason for hiding this comment

BtcContributor Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

ripcurlx commented Mar 17, 2021

wallclockbuilder commented Mar 17, 2021

sqrrm left a comment

Choose a reason for hiding this comment

sqrrm Mar 19, 2021

Choose a reason for hiding this comment

BtcContributor Mar 19, 2021

Choose a reason for hiding this comment

ripcurlx Apr 13, 2021

Choose a reason for hiding this comment

BtcContributor Apr 13, 2021

Choose a reason for hiding this comment

ripcurlx Apr 14, 2021

Choose a reason for hiding this comment

ripcurlx left a comment

Choose a reason for hiding this comment

BtcContributor commented Apr 19, 2021 • edited Loading

ripcurlx left a comment

Choose a reason for hiding this comment

mjtic12 commented May 9, 2021

BtcContributor commented May 16, 2021

mjtic12 commented May 16, 2021

BtcContributor commented May 17, 2021 • edited Loading

BtcContributor commented Mar 15, 2021 •

edited

Loading

BtcContributor Mar 15, 2021 •

edited

Loading

BtcContributor commented Apr 19, 2021 •

edited

Loading

BtcContributor commented May 17, 2021 •

edited

Loading